-
Notifications
You must be signed in to change notification settings - Fork 1.6k
refactor recursive reentrance checks #12349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.: - Implements the more restrictive lift/lower rules described in WebAssembly/component-model#589 such that a component instance may not lower a function lifted by one of its ancestors, nor vice-versa. Any such lower will result in a fused adapter which traps unconditionally, preventing guest-to-guest recursive reentrance without requiring data flow analysis. - Note that this required updating several WAST tests which were violating the new rule, including some in the `tests/component-model` Git submodule, which I've updated. - This is handled entirely in the `fact` module now; I've removed the `AlwaysTrap` case previously handled by `wasmtime-cranelift`. - Removes `FLAG_MAY_ENTER` from `InstanceFlags`. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on either `FLAG_NEEDS_POST_RETURN` for sync-lifted functions or the `GuestTask` call stack for async-lifted functions. - Adds a `StoreOpaque::trapped` field which is set when _any_ instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances. Note that this does _not_ include code to push and pop `GuestTask` instances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronous `Func::call` API, so certain intrinsics which expect a `GuestTask` to be present such as `backpressure.inc` will still fail in such cases. I'll address that in a later PR. Also note that I made a small change to `wasmtime-wit-bindgen`, adding a `Send` bound on the `T` type parameter for `store | async` functions. This allowed me to recursively call `{Typed}Func::call_concurrent` from inside a host function, and it doesn't have any downsides AFAICT. Fixes bytecodealliance#12128
alexcrichton
approved these changes
Jan 14, 2026
Member
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments here and there, but overall looks great, thanks!
deb61e1 to
a60e7f7
Compare
...and add const assertions to `trap.rs` to help avoid future divergence.
c0024b8 to
1ffadaa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit makes a few changes related to recursive reentrance checks, instance poisoning, etc.:
tests/component-modelGit submodule, which I've updated.factmodule now; I've removed theAlwaysTrapcase previously handled bywasmtime-cranelift.FLAG_MAY_ENTERfromInstanceFlags. It is no longer needed for guest-to-guest calls due to the above, and for guest-to-host-to-guest calls we can rely on eitherFLAG_NEEDS_POST_RETURNfor sync-lifted functions or theGuestTaskcall stack for async-lifted functions.StoreOpaque::trappedfield which is set when any instance belonging to that store traps, at which point the entire store is considered poisoned, meaning no instance belonging to it may be entered. This prevents indeterminant concurrent task state left over from the trapping instance from leaking into other instances.Note that this does not include code to push and pop
GuestTaskinstances for guest-to-guest sync-to-sync calls, nor for host-to-guest calls using e.g. the synchronousFunc::callAPI, so certain intrinsics which expect aGuestTaskto be present such asbackpressure.incwill still fail in such cases. I'll address that in a later PR.Also note that I made a small change to
wasmtime-wit-bindgen, adding aSendbound on theTtype parameter forstore | asyncfunctions. This allowed me to recursively call{Typed}Func::call_concurrentfrom inside a host function, and it doesn't have any downsides AFAICT.Fixes #12128